Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use the default value for a TopologyOption if omitted #3165

Merged
merged 2 commits into from
May 28, 2018
Merged

Conversation

bboreham
Copy link
Collaborator

@bboreham bboreham commented May 9, 2018

Without this, we see unmanaged, uncontained or not-running containers in the graph until the front-end requests them to go away.

There was a shouty comment saying not to use the default, but I can't see any reason to take that stance. There is only one place where "" is a valid value, which is namespaces, and it is also the default there.

To be safe, I changed the code to distinguish a lack of value from "" so you can have both.

@foot foot assigned foot and unassigned foot May 23, 2018
@foot foot self-requested a review May 23, 2018 07:01
@foot
Copy link
Contributor

foot commented May 23, 2018

This seems like a reasonable change in behaviour, but the scope-ui always fills in the defaults provided by /api/topologies before requesting the nodes so I don't think this will fix the linked issue.

@bboreham
Copy link
Collaborator Author

Here's a screen grab of it going wrong. I click on 'Deploy', start recording, click on 'Explore'.
The url is missing pseudo=hide

image

Copy link
Contributor

@foot foot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, thanks for the clean the repro. It does indeed fail to send the defaults. sigh.

Yeah, looks like it'll fix it.

@foot foot merged commit 4647ce1 into master May 28, 2018
@foot foot deleted the default-filter branch May 28, 2018 12:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants